Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixing Docker CI for security enabled tests #710

Merged
merged 6 commits into from
Nov 7, 2022

Conversation

amitgalitz
Copy link
Member

@amitgalitz amitgalitz commented Oct 28, 2022

Description

Split our CI into 3 different workflows.

  1. CI.yml -> This includes building AD, which includes UT and IT plus running multi node integration tests (linux, macos, windows). Windows is seperate here because I was not able to run it correctly with the -Dtest.logs=true option, only on linux and macos, I believe its okay for now as we only need that part for precision and recall and its enough to just see it on those two platforms.

  2. Security-Test-CI.yml -> Assembles AD and sets up docker container to run AD integ tests with security plugin if its available. (Linux only cause no infra set up yet for windows and macos)

  3. Bwc-test-CI.yml -> Runs BWC separately for AD (Linux only cause we don't have an older version with windows and macos)

Additionally it might be hard to notice cause of the new files but I changed some of the parsing logic in security-test-ci as it was failing to parse the correct docker version before.

Issues Resolved

resolves #275

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Amit Galitzky <amgalitz@amazon.com>
Signed-off-by: Amit Galitzky <amgalitz@amazon.com>
Signed-off-by: Amit Galitzky <amgalitz@amazon.com>
Signed-off-by: Amit Galitzky <amgalitz@amazon.com>
@opensearch-trigger-bot opensearch-trigger-bot bot added backport 2.x infra Changes to infrastructure, testing, CI/CD, pipelines, etc. labels Oct 28, 2022
@amitgalitz
Copy link
Member Author

Security tests are failing here, I think this is a 3.0 specific problem as the same tests passed on this PR for 2.x: #707
The errors are also related to http client which had some changes in 3.0 branch.
If rest of checks pass I believe we should still merge this in, backport the change and open an issue on security to further investigate errors.

@amitgalitz amitgalitz marked this pull request as ready for review October 28, 2022 21:32
@amitgalitz amitgalitz requested review from a team, jackiehanyang and ohltyler October 28, 2022 21:32
@ohltyler
Copy link
Member

Can we rename the workflows to follow a consistent convention? I think all underscore (verb-first) like mentioned here is a good way, or all--. I see our workflows are already a bit inconsistent

@codecov-commenter
Copy link

codecov-commenter commented Oct 28, 2022

Codecov Report

Merging #710 (7b897bd) into main (06e5edb) will decrease coverage by 0.17%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #710      +/-   ##
============================================
- Coverage     79.26%   79.08%   -0.18%     
+ Complexity     4260     4243      -17     
============================================
  Files           301      301              
  Lines         17872    17872              
  Branches       1897     1897              
============================================
- Hits          14166    14134      -32     
- Misses         2810     2835      +25     
- Partials        896      903       +7     
Flag Coverage Δ
plugin 79.08% <ø> (-0.18%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ava/org/opensearch/ad/task/ADHCBatchTaskCache.java 90.12% <0.00%> (-2.47%) ⬇️
...java/org/opensearch/ad/task/ADBatchTaskRunner.java 82.21% <0.00%> (-2.44%) ⬇️
...ain/java/org/opensearch/ad/task/ADTaskManager.java 76.90% <0.00%> (-1.06%) ⬇️
.../main/java/org/opensearch/ad/cluster/HashRing.java 81.37% <0.00%> (-0.41%) ⬇️
...ain/java/org/opensearch/ad/model/ModelProfile.java 72.72% <0.00%> (+1.81%) ⬆️

Signed-off-by: Amit Galitzky <amgalitz@amazon.com>
Signed-off-by: Amit Galitzky <amgalitz@amazon.com>
@amitgalitz
Copy link
Member Author

More context on security failures here: #712

@amitgalitz amitgalitz merged commit bcfe952 into opensearch-project:main Nov 7, 2022
opensearch-trigger-bot bot pushed a commit that referenced this pull request Nov 7, 2022
Signed-off-by: Amit Galitzky <amgalitz@amazon.com>
(cherry picked from commit bcfe952)
ohltyler pushed a commit that referenced this pull request Nov 8, 2022
Signed-off-by: Amit Galitzky <amgalitz@amazon.com>
(cherry picked from commit bcfe952)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x infra Changes to infrastructure, testing, CI/CD, pipelines, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add MacOS Support
4 participants